Refactor badges update job: modularize handlers, introduce base class, and add tests#3040
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors badge-sync into a reusable BaseBadgeCommand, removes the old monolithic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-12-31T05:17:39.659ZApplied to files:
📚 Learning: 2026-01-01T17:48:23.963ZApplied to files:
🧬 Code graph analysis (1)backend/apps/nest/models/user_badge.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/apps/nest/badges/project_leader_badge.py (1)
28-47: Consider caching the ContentType lookup.The eligibility logic is correct and comprehensive. The filters appropriately identify active, reviewed project leaders with linked user accounts.
For improved efficiency, consider caching
ContentType.objects.get_for_model(Project)as a class attribute, since it's called every timeget_eligible_users()runs and always returns the same value.🔎 Suggested optimization
At the class level, add:
class OWASPProjectLeaderBadgeHandler(BaseBadgeHandler): """Handler for managing the OWASP Project Leader badge. This badge is awarded to users who are active and reviewed leaders of OWASP projects. It uses the EntityMember model for users with the LEADER role. """ name = "OWASP Project Leader" description = "Official OWASP Project Leader" css_class = "fa-user-shield" weight = 90 # Cache the content type at class level _project_content_type = None @classmethod def get_project_content_type(cls): if cls._project_content_type is None: cls._project_content_type = ContentType.objects.get_for_model(Project) return cls._project_content_typeThen update the method:
def get_eligible_users(self) -> QuerySet[User]: # Get IDs of users who are active and reviewed project leaders leader_ids = EntityMember.objects.filter( - entity_type=ContentType.objects.get_for_model(Project), + entity_type=self.get_project_content_type(), role=EntityMember.Role.LEADER, is_active=True, is_reviewed=True, member__isnull=False, ).values_list("member_id", flat=True) return User.objects.filter(id__in=leader_ids).distinct()backend/apps/nest/badges/base.py (1)
75-81: Consider bulk operations for better performance.The current implementation uses individual
get_or_createandsavecalls in a loop. For large user bases, this could be optimized using bulk operations to reduce database round-trips.💡 Potential optimization approach
# Get existing UserBadge records existing_user_badges = UserBadge.objects.filter( user__in=users_to_add, badge=badge ).select_related('user') existing_map = {ub.user_id: ub for ub in existing_user_badges} # Prepare lists for bulk operations to_create = [] to_update = [] for user in users_to_add: if user.id in existing_map: user_badge = existing_map[user.id] if not user_badge.is_active: user_badge.is_active = True to_update.append(user_badge) else: to_create.append(UserBadge(user=user, badge=badge, is_active=True)) # Bulk create and update if to_create: UserBadge.objects.bulk_create(to_create) if to_update: UserBadge.objects.bulk_update(to_update, ['is_active']) added_count = len(to_create) + len(to_update)Note: This optimization is optional and can be deferred if the current performance is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/nest/badges/__init__.pybackend/apps/nest/badges/base.pybackend/apps/nest/badges/project_leader_badge.pybackend/apps/nest/badges/staff_badge.pybackend/apps/nest/management/commands/nest_update_badges.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-01T04:15:32.151Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1823
File: frontend/__tests__/e2e/pages/Login.spec.ts:28-34
Timestamp: 2025-08-01T04:15:32.151Z
Learning: In the OWASP Nest project, the login page (/auth/login) handles only authentication (GitHub OAuth) and does not differentiate between OWASP staff and non-staff users. The role-based access control using the is_owasp_staff field happens after authentication in downstream components like DashboardWrapper and ProjectsWrapper, not during the login process itself.
Applied to files:
backend/apps/nest/badges/staff_badge.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/nest/management/commands/nest_update_badges.py
🧬 Code graph analysis (4)
backend/apps/nest/badges/__init__.py (3)
backend/apps/github/api/internal/nodes/user.py (1)
badges(37-51)backend/apps/nest/badges/project_leader_badge.py (1)
OWASPProjectLeaderBadgeHandler(16-47)backend/apps/nest/badges/staff_badge.py (1)
OWASPStaffBadgeHandler(13-28)
backend/apps/nest/management/commands/nest_update_badges.py (4)
backend/apps/github/api/internal/nodes/user.py (1)
badges(37-51)backend/apps/nest/badges/project_leader_badge.py (1)
OWASPProjectLeaderBadgeHandler(16-47)backend/apps/nest/badges/staff_badge.py (1)
OWASPStaffBadgeHandler(13-28)backend/apps/nest/badges/base.py (1)
process(47-97)
backend/apps/nest/badges/project_leader_badge.py (2)
backend/apps/nest/badges/base.py (2)
BaseBadgeHandler(13-97)get_eligible_users(26-30)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)
backend/apps/nest/badges/base.py (3)
backend/apps/nest/models/user_badge.py (1)
UserBadge(10-50)backend/apps/nest/badges/project_leader_badge.py (1)
get_eligible_users(28-47)backend/apps/nest/badges/staff_badge.py (1)
get_eligible_users(21-28)
🪛 Ruff (0.14.10)
backend/apps/nest/badges/__init__.py
6-9: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
backend/apps/nest/badges/staff_badge.py
22-27: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
25-25: Missing blank line after last section ("Returns")
Add blank line after "Returns"
(D413)
backend/apps/nest/badges/project_leader_badge.py
29-37: No blank lines allowed after function docstring (found 1)
Remove blank line(s) after function docstring
(D202)
29-37: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
35-35: Missing blank line after last section ("Returns")
Add blank line after "Returns"
(D413)
backend/apps/nest/badges/base.py
1-1: Missing docstring in public module
(D100)
21-21: Missing docstring in __init__
(D107)
27-29: One-line docstring should fit on one line
Reformat to one line
(D200)
27-29: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
30-30: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
32-32: Missing docstring in public method
(D102)
40-40: First line of docstring should be in imperative mood: "Helper to log to both file logger and stdout if available."
(D401)
48-53: 1 blank line required between summary line and description
(D205)
48-53: Multi-line docstring summary should start at the first line
Remove whitespace after opening quotes
(D212)
48-53: First line of docstring should be in imperative mood: "Main execution method to sync the badge."
(D401)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
97-97: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (5)
backend/apps/nest/badges/staff_badge.py (2)
13-19: LGTM!The class definition and badge metadata are correctly structured. The weight of 100 appropriately positions the staff badge with high priority.
21-28: LGTM!The eligibility logic correctly filters users by the
is_owasp_stafffield, which aligns with the project's role-based access control approach.backend/apps/nest/badges/__init__.py (1)
1-9: LGTM!The package initialization correctly exposes the two badge handlers, establishing a clean public API for the badges module.
backend/apps/nest/badges/project_leader_badge.py (1)
16-26: LGTM!The class definition and badge metadata are well-structured. The weight of 90 appropriately positions the project leader badge below the staff badge.
backend/apps/nest/management/commands/nest_update_badges.py (1)
20-37: Excellent refactoring with proper error isolation!The new pluggable handler architecture cleanly separates concerns and improves maintainability. The per-handler try/except ensures that a failure in one badge type doesn't prevent other badges from being processed. Error logging and styled output align with Django management command best practices.
Based on learnings, the use of
self.stdout.writeandself.style.SUCCESS/ERRORis appropriate for Django management commands.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
backend/tests/apps/nest/badges/staff_badge_test.py (1)
10-18: Add trailing newline and consider asserting the return value.The test verifies the filter is called correctly but doesn't assert that
get_eligible_users()returns the expected queryset. Consider adding an assertion for completeness.🔎 Proposed enhancement
@patch("apps.nest.badges.staff_badge.User") def test_get_eligible_users_filters_staff(self, mock_user_model): """Test that get_eligible_users filters for is_owasp_staff=True.""" handler = OWASPStaffBadgeHandler() - handler.get_eligible_users() + result = handler.get_eligible_users() mock_user_model.objects.filter.assert_called_once_with(is_owasp_staff=True) + assert result == mock_user_model.objects.filter.return_value +backend/tests/apps/nest/management/commands/nest_update_badges_test.py (1)
76-86: Add trailing newline.The test correctly verifies exception logging with stack traces. Minor: add a trailing newline at end of file.
🔎 Proposed fix
mock_handlers_dict.keys.return_value = ["staff"] call_command("nest_update_badges", stdout=StringIO()) mock_logger.exception.assert_called_with("Error processing badge %s", "OWASP Staff") +backend/tests/apps/nest/badges/project_leader_badge_test.py (2)
7-8: Remove unused import.
EntityMemberis imported but not used in the test. The assertion on line 38 usesmock_entity_member.Role.LEADER(the patched mock), not the actual import.🔎 Proposed fix
from apps.nest.badges.project_leader_badge import OWASPProjectLeaderBadgeHandler -from apps.owasp.models.entity_member import EntityMember
43-44: Add trailing newline.🔎 Proposed fix
mock_user_model.objects.filter.assert_called_once_with(id__in=[101, 102]) mock_user_model.objects.filter.return_value.distinct.assert_called_once() +backend/apps/nest/management/commands/nest_update_badges.py (1)
43-52: Consider differentiating success message when handlers fail.The "User badges sync completed" message prints unconditionally, even if all handlers failed. This could be misleading in error scenarios.
🔎 Proposed enhancement
+ failed_count = 0 for handler_class in handlers_to_run: try: self.stdout.write(f"Processing badge: {handler_class.name}") handler = handler_class(stdout=self.stdout, style=self.style) handler.process() except Exception: logger.exception("Error processing badge %s", handler_class.name) self.stdout.write(self.style.ERROR(f"Failed to update {handler_class.name}")) + failed_count += 1 - self.stdout.write(self.style.SUCCESS("User badges sync completed")) + if failed_count: + self.stdout.write( + self.style.WARNING(f"User badges sync completed with {failed_count} failure(s)") + ) + else: + self.stdout.write(self.style.SUCCESS("User badges sync completed"))backend/tests/apps/nest/badges/base_test.py (2)
11-19: Add blank line after class docstring.Per PEP 257 convention (flagged by static analysis D204).
🔎 Proposed fix
class ConcreteTestHandler(BaseBadgeHandler): """Concrete implementation of BaseBadgeHandler for testing purposes.""" + name = "Test Badge" description = "Test Description"
75-95: Add trailing newline; tests look good.The revocation test correctly verifies the bulk update flow. Add trailing newline at end of file.
🔎 Proposed fix
mock_revocation_qs.update.assert_called_with(is_active=False) self.assertIn("Removed 'Test Badge' badge from 5 users", self.out.getvalue()) +
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/apps/nest/Makefilebackend/apps/nest/badges/__init__.pybackend/apps/nest/badges/base.pybackend/apps/nest/badges/project_leader_badge.pybackend/apps/nest/badges/staff_badge.pybackend/apps/nest/management/commands/nest_update_badges.pybackend/tests/apps/nest/badges/base_test.pybackend/tests/apps/nest/badges/project_leader_badge_test.pybackend/tests/apps/nest/badges/staff_badge_test.pybackend/tests/apps/nest/management/commands/nest_update_badges_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/apps/nest/badges/staff_badge.py
- backend/apps/nest/badges/init.py
- backend/apps/nest/badges/base.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-06T19:03:01.985Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/common.py:213-232
Timestamp: 2025-09-06T19:03:01.985Z
Learning: In the OWASP Nest project, the get_leaders_emails() method in RepositoryBasedEntityModel is designed to only capture leaders with mailto: links from leaders.md files, intentionally ignoring plain text names without email addresses. The current regex implementation works correctly for the intended behavior as validated by comprehensive test cases.
Applied to files:
backend/apps/nest/badges/project_leader_badge.pybackend/tests/apps/nest/badges/project_leader_badge_test.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/tests/apps/nest/management/commands/nest_update_badges_test.pybackend/apps/nest/management/commands/nest_update_badges.py
🧬 Code graph analysis (5)
backend/apps/nest/badges/project_leader_badge.py (3)
backend/apps/nest/badges/base.py (2)
BaseBadgeHandler(15-86)get_eligible_users(29-30)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/nest/badges/staff_badge.py (1)
get_eligible_users(17-24)
backend/tests/apps/nest/management/commands/nest_update_badges_test.py (3)
backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)backend/apps/nest/badges/base.py (1)
process(48-86)backend/tests/apps/github/management/commands/github_update_related_organizations_test.py (1)
mock_logger(18-22)
backend/tests/apps/nest/badges/base_test.py (1)
backend/apps/nest/badges/base.py (3)
BaseBadgeHandler(15-86)get_eligible_users(29-30)process(48-86)
backend/apps/nest/management/commands/nest_update_badges.py (5)
backend/apps/github/api/internal/nodes/user.py (1)
badges(37-51)backend/apps/nest/badges/project_leader_badge.py (1)
OWASPProjectLeaderBadgeHandler(12-34)backend/apps/nest/badges/staff_badge.py (1)
OWASPStaffBadgeHandler(9-24)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)backend/apps/nest/badges/base.py (1)
process(48-86)
backend/tests/apps/nest/badges/project_leader_badge_test.py (4)
backend/apps/nest/badges/project_leader_badge.py (1)
get_eligible_users(20-34)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/nest/badges/base.py (1)
get_eligible_users(29-30)backend/tests/apps/nest/badges/base_test.py (1)
get_eligible_users(18-19)
🪛 Ruff (0.14.10)
backend/tests/apps/nest/badges/staff_badge_test.py
1-1: File backend/tests/apps/nest/badges/staff_badge_test.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
18-18: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/nest/badges/base_test.py
1-1: File backend/tests/apps/nest/badges/base_test.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
12-12: 1 blank line required after class docstring
Insert 1 blank line after class docstring
(D204)
50-50: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
73-73: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
95-95: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
95-95: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/nest/badges/project_leader_badge_test.py
1-1: File backend/tests/apps/nest/badges/project_leader_badge_test.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
44-44: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (7)
backend/apps/nest/Makefile (1)
1-3: LGTM!The addition of
$(ARGS)enables passing the new--handlerargument to the management command, aligning with the refactored handler-driven architecture.backend/tests/apps/nest/management/commands/nest_update_badges_test.py (2)
13-34: LGTM!Good test coverage for the all-handlers scenario. The mocking approach correctly isolates the handler instantiation and process calls.
52-74: LGTM!Excellent test for error resilience - verifies that a failing handler doesn't prevent subsequent handlers from executing, and that appropriate error messages are written to stdout.
backend/apps/nest/badges/project_leader_badge.py (1)
12-34: LGTM!The implementation correctly queries for active, reviewed project leaders with linked user accounts. The
distinct()is a good defensive measure.One minor consideration:
ContentType.objects.get_for_model(Project)is called on every invocation. Django caches ContentType lookups internally, so this is fine for the current use case.backend/apps/nest/management/commands/nest_update_badges.py (1)
17-20: LGTM!Clean handler registry pattern that makes adding new badge handlers straightforward. The keys provide intuitive CLI identifiers.
backend/tests/apps/nest/badges/base_test.py (2)
28-50: LGTM!Good coverage of badge creation flow. Verifies
get_or_createis called with correct defaults and that the creation message is logged.
52-73: LGTM!Thorough test of the badge assignment flow - correctly verifies UserBadge creation, activation toggle, and save with
update_fields.
mrkeshav-05
force-pushed
the
refactor/nest-badges
branch
from
4bab2d4 to
30ac8a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/apps/nest/badges/staff_badge_test.py (1)
1-19: Consider adding__init__.pyfor explicit test package structure.The test directory
backend/tests/apps/nest/badges/would benefit from an explicit__init__.pyfile to ensure consistent test discovery across different test runners and Python environments, though implicit namespace packages work in Python 3.3+.📝 Create `__init__.py` in test directories
Create an empty
backend/tests/apps/nest/badges/__init__.py:"""Tests for nest badge handlers."""Apply this to all intermediate test directories if not already present:
backend/tests/apps/nest/badges/__init__.py
Test implementation looks solid.
The test correctly verifies that
get_eligible_userscallsUser.objects.filterwith the expected parameter and returns the filtered queryset.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/nest/management/commands/nest_update_badges.pybackend/tests/apps/nest/badges/base_test.pybackend/tests/apps/nest/badges/project_leader_badge_test.pybackend/tests/apps/nest/badges/staff_badge_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/nest/management/commands/nest_update_badges.py
🧬 Code graph analysis (3)
backend/apps/nest/management/commands/nest_update_badges.py (3)
backend/apps/nest/badges/project_leader_badge.py (1)
OWASPProjectLeaderBadgeHandler(12-34)backend/apps/nest/badges/staff_badge.py (1)
OWASPStaffBadgeHandler(9-24)backend/apps/nest/badges/base.py (1)
process(48-86)
backend/tests/apps/nest/badges/base_test.py (3)
backend/apps/github/api/internal/nodes/user.py (1)
badges(37-51)backend/apps/nest/badges/base.py (3)
BaseBadgeHandler(15-86)get_eligible_users(29-30)process(48-86)backend/apps/nest/badges/staff_badge.py (1)
get_eligible_users(17-24)
backend/tests/apps/nest/badges/project_leader_badge_test.py (2)
backend/apps/nest/badges/project_leader_badge.py (2)
OWASPProjectLeaderBadgeHandler(12-34)get_eligible_users(20-34)backend/apps/owasp/models/entity_member.py (1)
Role(13-16)
🪛 Ruff (0.14.10)
backend/tests/apps/nest/badges/base_test.py
1-1: File backend/tests/apps/nest/badges/base_test.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
51-51: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
74-74: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
96-96: Use a regular assert instead of unittest-style assertIn
Replace assertIn(...) with assert ...
(PT009)
backend/tests/apps/nest/badges/project_leader_badge_test.py
1-1: File backend/tests/apps/nest/badges/project_leader_badge_test.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
backend/tests/apps/nest/badges/staff_badge_test.py
1-1: File backend/tests/apps/nest/badges/staff_badge_test.py is part of an implicit namespace package. Add an __init__.py.
(INP001)
🔇 Additional comments (7)
backend/tests/apps/nest/badges/project_leader_badge_test.py (1)
1-43: Comprehensive test coverage for project leader badge logic.The test thoroughly validates the multi-step query process:
- ContentType resolution for the Project model
- EntityMember filtering with correct role, status, and relationship checks
- User filtering by leader IDs with distinct() applied
The mock setup and assertions align well with the handler implementation.
backend/apps/nest/management/commands/nest_update_badges.py (2)
17-30: Well-designed pluggable handler registry.The
BADGE_HANDLERSdictionary provides a clean extension point for adding new badge types. The--handlerargument withchoicesvalidation ensures only valid handlers can be selected, while allowing all handlers to run when omitted.
32-61: Robust handler execution with proper error handling.The implementation correctly:
- Selects handlers based on CLI argument
- Instantiates each handler with stdout and style for output formatting
- Isolates failures with per-handler try/except blocks so one failure doesn't abort the entire run
- Uses
logger.exceptionto capture full tracebacks- Reports final status with failure count
The use of
self.stdout.write()andself.stylemethods aligns with Django management command conventions. Based on learnings, this is the preferred approach overprint().backend/tests/apps/nest/badges/base_test.py (4)
11-27: Solid test infrastructure for abstract base class.The
ConcreteTestHandlerprovides a minimal concrete implementation suitable for testing the base class logic, and thesetUpmethod correctly initializes output capture for verifying_logcalls.
29-51: Badge creation test correctly validates core behavior.The test verifies that
process()callsBadge.objects.get_or_createwith the correct badge name and defaults (description, css_class, weight), and confirms the creation message is logged.
53-74: Badge assignment test validates activation logic.The test correctly verifies that
process():
- Creates or retrieves
UserBadgefor eligible users- Activates inactive badges by setting
is_active=True- Saves with
update_fieldsfor efficiency- Logs the count of added badges
76-96: Badge revocation test validates cleanup logic.The test correctly verifies that
process():
- Identifies
UserBadgerecords for users no longer eligible- Bulk updates them with
is_active=False- Logs the count of revoked badges
The use of
assertInis appropriate forSimpleTestCase(unittest-based tests). The static analysis suggestion to useassertinstead applies to pytest-style tests, not Django's unittest framework.
mrkeshav-05
force-pushed
the
refactor/nest-badges
branch
from
30ac8a0 to
e7132f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/apps/nest/badges/base_test.py (1)
54-77: Consider mocking the revocation branch for better test isolation.The test correctly validates the badge addition flow. However, the revocation branch (
UserBadge.objects.filter(...).exclude(...)) is not explicitly mocked. Due to MagicMock's default behavior, this branch will execute with a truthycount()value, causing unintended log output during the test.While the test passes because the assertion only checks for the "Added" message, explicitly mocking the revocation path would make the test more precise.
🔎 Optional: Add explicit mock for revocation path
mock_user_badge = MagicMock() mock_user_badge.is_active = False mock_user_badge_model.objects.get_or_create.return_value = (mock_user_badge, False) + + # Mock revocation path to return no users to revoke + mock_revocation_qs = MagicMock() + mock_revocation_qs.count.return_value = 0 + mock_user_badge_model.objects.filter.return_value.exclude.return_value = mock_revocation_qs + self.handler.process()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/nest/management/commands/nest_update_badges.pybackend/tests/apps/nest/badges/__init__.pybackend/tests/apps/nest/badges/base_test.pybackend/tests/apps/nest/badges/project_leader_badge_test.pybackend/tests/apps/nest/badges/staff_badge_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/nest/badges/project_leader_badge_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/nest/management/commands/nest_update_badges.py
🧬 Code graph analysis (3)
backend/apps/nest/management/commands/nest_update_badges.py (3)
backend/apps/nest/badges/project_leader_badge.py (1)
OWASPProjectLeaderBadgeHandler(12-34)backend/apps/nest/badges/staff_badge.py (1)
OWASPStaffBadgeHandler(9-24)backend/apps/nest/badges/base.py (1)
process(48-86)
backend/tests/apps/nest/badges/base_test.py (1)
backend/apps/nest/badges/base.py (3)
BaseBadgeHandler(15-86)get_eligible_users(29-30)process(48-86)
backend/tests/apps/nest/badges/staff_badge_test.py (4)
backend/apps/nest/badges/staff_badge.py (2)
OWASPStaffBadgeHandler(9-24)get_eligible_users(17-24)backend/tests/apps/nest/badges/base_test.py (1)
get_eligible_users(19-20)backend/apps/nest/badges/base.py (1)
get_eligible_users(29-30)backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-16)
🔇 Additional comments (6)
backend/tests/apps/nest/badges/staff_badge_test.py (1)
10-18: LGTM!Clean and focused test that correctly validates the core filtering logic. The patch target path is correct (patching where the
Useris imported in the module under test), and the assertion properly verifies both the filter call arguments and the return value.backend/apps/nest/management/commands/nest_update_badges.py (2)
44-52: Robust error handling with continuation.Good pattern: catching exceptions per-handler allows other handlers to proceed even if one fails. Using
logger.exceptioncaptures the full traceback for debugging.One minor consideration: if a handler class doesn't define
name(violating the base class contract), accessinghandler_class.nameon line 46 could raiseAttributeErrorbeforeprocess()is called. However, since the handlers are well-defined in this PR and the base class enforcesnamevia its contract, this is acceptable.
32-41: LGTM!Clean implementation of the handler selection logic. Using
self.stdout.writealigns with Django management command conventions. Based on learnings, this approach improves testability compared to usingprint().backend/tests/apps/nest/badges/base_test.py (3)
30-52: LGTM!Good test coverage for the badge creation path. Correctly verifies:
get_or_createis called with the expected name and defaults- The "Created badge" log message appears when a new badge is created
79-101: LGTM!Good test coverage for the revocation path. The mock setup correctly chains
filter().exclude()and verifies theupdate(is_active=False)call.
11-20: LGTM!Clean concrete test handler implementation that satisfies the abstract base class contract. Defining all required attributes (
name,description,css_class,weight) ensures theprocess()method can execute without raising validation errors.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Actually, I expected it to have a management command base and derived per badge management commands. What do you think about this refactoring?
| ] | ||
|
|
||
| if new_badges: | ||
| UserBadge.objects.bulk_create( |
There was a problem hiding this comment.
This class has BulkSaveModel functionality.
| added_count = len(new_badges) | ||
|
|
||
| if added_count > 0: | ||
| user_word = "user" if added_count == 1 else "users" |
There was a problem hiding this comment.
This is not consistent with existing cases.
|
|
||
| added_count = len(new_badges) | ||
|
|
||
| if added_count > 0: |
There was a problem hiding this comment.
You didn't learn from previous review rounds.
| if added_count > 0: | |
| if added_count: |
| unique_fields=["user", "badge"], | ||
| ) | ||
|
|
||
| added_count = len(new_badges) |
There was a problem hiding this comment.
You can combine those ifs
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/nest/management/commands/base_badge_command.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/nest/management/commands/base_badge_command.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/apps/nest/management/commands/base_badge_command.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/apps/nest/management/commands/base_badge_command.py
🧬 Code graph analysis (1)
backend/apps/nest/management/commands/base_badge_command.py (3)
backend/apps/nest/models/user_badge.py (1)
UserBadge(10-50)backend/apps/nest/management/commands/nest_update_project_leader_badge.py (1)
get_eligible_users(20-28)backend/apps/nest/management/commands/nest_update_staff_badge.py (1)
get_eligible_users(17-18)
🔇 Additional comments (8)
backend/apps/nest/management/commands/base_badge_command.py (8)
1-14: LGTM: Clean imports and setup.The imports and logger configuration follow Django best practices. The use of ABC for the abstract base class is appropriate for this modular design.
16-23: LGTM: Well-structured base class design.The class-level attributes provide a clean interface for subclasses to declare badge metadata. The use of optional types (
str | None) correctly indicates these must be overridden by concrete implementations.
24-26: LGTM: Abstract method enforces the template pattern.The abstract
get_eligible_users()method correctly forces subclasses to implement badge-specific eligibility logic, which is the core goal of this refactoring.
28-30: LGTM: Logging follows Django conventions.The dual logging to both
logger.infoandself.stdout.writeis appropriate for Django management commands, aligning with retrieved learnings about command output patterns.
32-37: LGTM: Defensive validation.The validation ensures subclasses properly configure
badge_name, providing a clear error message if not. The initial sync message provides good user feedback.
39-56: LGTM: Metadata syncing correctly implemented.The code now properly updates existing badge metadata (lines 52-55) in addition to setting defaults on creation. This ensures the "sync" command truly synchronizes badge properties on every run. The use of
update_fieldsis efficient. ✅ Addresses past review feedback.
78-88: LGTM: Badge removal logic is correct.The query correctly identifies badges to deactivate, and the bulk update with
is_active=Falseis efficient. Pluralization is handled properly for user feedback.
89-93: LGTM: Proper exception handling and success messaging.The exception handling logs full details (with traceback via
logger.exception) while providing styled output to users. Re-raising the exception allows calling code to handle failures appropriately.
mrkeshav-05
force-pushed
the
refactor/nest-badges
branch
from
5699b90 to
a5dffd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/apps/nest/models/user_badge.py:
- Around line 52-55: The docstring on the static method bulk_save incorrectly
says "Bulk save organizations."; update the docstring for bulk_save in the
UserBadge class to accurately describe its behavior (e.g., "Bulk save user
badges.") so it references user badges rather than organizations; ensure this
change is applied to the bulk_save method that calls
BulkSaveModel.bulk_save(UserBadge, user_badges, fields=fields).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/generated_videos/community_snapshot_video_2025/2025_snapshot.mkvis excluded by!**/*.mkv
📒 Files selected for processing (9)
backend/apps/github/models/organization.pybackend/apps/nest/Makefilebackend/apps/nest/management/commands/base_badge_command.pybackend/apps/nest/management/commands/nest_update_project_leader_badges.pybackend/apps/nest/management/commands/nest_update_staff_badges.pybackend/apps/nest/models/user_badge.pybackend/tests/apps/github/models/organization_test.pybackend/tests/apps/nest/management/commands/nest_update_project_leader_badges_test.pybackend/tests/apps/nest/management/commands/nest_update_staff_badges_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/nest/management/commands/base_badge_command.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/apps/nest/models/user_badge.pybackend/apps/nest/management/commands/nest_update_project_leader_badges.pybackend/tests/apps/github/models/organization_test.pybackend/tests/apps/nest/management/commands/nest_update_project_leader_badges_test.pybackend/tests/apps/nest/management/commands/nest_update_staff_badges_test.pybackend/apps/nest/management/commands/nest_update_staff_badges.pybackend/apps/github/models/organization.py
📚 Learning: 2026-01-01T17:48:23.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:41-47
Timestamp: 2026-01-01T17:48:23.963Z
Learning: In Django code, be aware that a QuerySet's boolean evaluation (e.g., if not queryset) runs a database query to determine emptiness. While it is technically valid to use the queryset in a boolean context, use queryset.exists() for existence checks to avoid unnecessary queries and improve performance. Applicable broadly to Python/Django files rather than just this specific path.
Applied to files:
backend/apps/nest/models/user_badge.pybackend/apps/nest/management/commands/nest_update_project_leader_badges.pybackend/tests/apps/github/models/organization_test.pybackend/tests/apps/nest/management/commands/nest_update_project_leader_badges_test.pybackend/tests/apps/nest/management/commands/nest_update_staff_badges_test.pybackend/apps/nest/management/commands/nest_update_staff_badges.pybackend/apps/github/models/organization.py
📚 Learning: 2025-11-23T11:37:26.253Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2606
File: backend/apps/api/rest/v0/project.py:43-48
Timestamp: 2025-11-23T11:37:26.253Z
Learning: In the OWASP Nest backend, `entity_leaders` is a `property` method defined in `RepositoryBasedEntityModel` (backend/apps/owasp/models/common.py) that returns a dynamically constructed QuerySet. It cannot be prefetched using standard `prefetch_related()` because Django's prefetch mechanism only works on model fields and relations, not property methods.
Applied to files:
backend/apps/nest/management/commands/nest_update_project_leader_badges.py
📚 Learning: 2025-09-06T19:03:01.985Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/common.py:213-232
Timestamp: 2025-09-06T19:03:01.985Z
Learning: In the OWASP Nest project, the get_leaders_emails() method in RepositoryBasedEntityModel is designed to only capture leaders with mailto: links from leaders.md files, intentionally ignoring plain text names without email addresses. The current regex implementation works correctly for the intended behavior as validated by comprehensive test cases.
Applied to files:
backend/apps/nest/management/commands/nest_update_project_leader_badges.py
📚 Learning: 2025-12-18T05:39:42.678Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2948
File: backend/apps/owasp/management/commands/owasp_generate_community_snapshot_video.py:40-40
Timestamp: 2025-12-18T05:39:42.678Z
Learning: In Django management commands, prefer using self.stdout.write(...) over print(...) for user-facing stdout output. This aligns with Django conventions and improves testability. When emitting messages, consider using self.stdout.write and, for styled messages, use self.style.SUCCESS/ERROR as appropriate to maintain consistent command output formatting. Apply this guideline to all Python files within any project's management/commands directory.
Applied to files:
backend/apps/nest/management/commands/nest_update_project_leader_badges.pybackend/tests/apps/nest/management/commands/nest_update_project_leader_badges_test.pybackend/tests/apps/nest/management/commands/nest_update_staff_badges_test.pybackend/apps/nest/management/commands/nest_update_staff_badges.py
🧬 Code graph analysis (4)
backend/apps/nest/models/user_badge.py (3)
backend/apps/github/models/organization.py (1)
bulk_save(89-91)backend/apps/github/models/user.py (1)
bulk_save(167-169)backend/apps/common/models.py (1)
BulkSaveModel(10-34)
backend/tests/apps/github/models/organization_test.py (1)
backend/apps/github/models/organization.py (1)
Organization(20-115)
backend/tests/apps/nest/management/commands/nest_update_staff_badges_test.py (2)
backend/apps/nest/management/commands/nest_update_staff_badges.py (1)
Command(9-18)backend/tests/apps/nest/management/commands/nest_update_project_leader_badges_test.py (3)
test_has_correct_metadata(14-16)test_command_runs(24-44)test_handles_errors(52-59)
backend/apps/nest/management/commands/nest_update_staff_badges.py (2)
backend/apps/nest/management/commands/base_badge_command.py (2)
BaseBadgeCommand(17-90)get_eligible_users(26-27)backend/apps/nest/api/internal/nodes/user.py (1)
is_owasp_staff(14-16)
🪛 checkmake (0.2.2)
backend/apps/nest/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (10)
backend/tests/apps/github/models/organization_test.py (1)
15-15: LGTM!The test assertion correctly verifies that
Organization.bulk_saveforwards thefieldsparameter toBulkSaveModel.bulk_save.backend/apps/github/models/organization.py (1)
89-91: LGTM!The addition of the
fieldsparameter enables selective field updates during bulk operations while maintaining backward compatibility. The implementation correctly forwards the parameter toBulkSaveModel.bulk_save.backend/tests/apps/nest/management/commands/nest_update_staff_badges_test.py (1)
1-46: LGTM!The test suite provides comprehensive coverage of the staff badges command:
- Metadata verification ensures correct badge configuration
- Execution test validates the command's happy path with appropriate mocking
- Error handling test confirms exceptions propagate correctly
The use of
SimpleTestCaseis appropriate since all dependencies are mocked, and the testing pattern aligns well with the project leader badges tests.backend/apps/nest/Makefile (1)
1-11: LGTM!The refactoring successfully modularizes badge updates into separate targets while maintaining backward compatibility through the aggregate
nest-update-badgestarget. This design facilitates running individual badge updates independently and simplifies adding new badge types in the future.The static analysis warnings about missing phony targets (
all,clean,test) are false positives—this appears to be a partial Makefile included by a parent Makefile that defines those targets.backend/apps/nest/management/commands/nest_update_staff_badges.py (1)
9-15: Clean implementation of badge metadata.The command properly extends
BaseBadgeCommandand defines all required badge metadata. The weight of 100 (higher than project leader's 90) appropriately reflects the badge hierarchy.backend/apps/nest/management/commands/nest_update_project_leader_badges.py (2)
12-18: LGTM! Badge metadata is properly configured.The command correctly extends
BaseBadgeCommandwith appropriate metadata. The weight of 90 properly positions project leaders in the badge hierarchy below staff (100).
20-29: The suggested optimization cannot be implemented due to model constraints.The
EntityMember.memberForeignKey usesrelated_name="+"(line 63 ofbackend/apps/owasp/models/entity_member.py), which explicitly disables the reverse relation from User to EntityMember. The current implementation usingid__inwith a subquery is the appropriate approach and cannot be optimized using the suggested direct join pattern.Likely an incorrect or invalid review comment.
backend/tests/apps/nest/management/commands/nest_update_project_leader_badges_test.py (3)
13-16: LGTM! Clean metadata validation test.The test directly verifies the command's badge metadata configuration. Simple and effective.
18-44: Test appropriately validates command execution with mocked dependencies.The test properly mocks all external dependencies and verifies the command runs successfully with expected output. The extensive mocking is appropriate for a unit test that focuses on command integration rather than business logic (which is tested elsewhere in the base class tests).
46-59: LGTM! Error handling test properly validates exception propagation.The test correctly simulates a failure in
get_eligible_usersand verifies that the exception propagates as expected. This validates the error handling behavior defined inBaseBadgeCommand.
|
…, and add tests (OWASP#3040) * update nest badges code * update tests in nest badge * apply coderabbit suggestions * Refactor badges with base command * resolve sonarcloud issues * Update code * apply bulk save and pluralize log messages * update code * Update code * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
2 participants
Proposed change
This PR modularises the badge update with
BaseBadgeHandlerto share logic and separate handler files for each badge type.Resolves: #3019
Checklist
make check-testlocally and all tests passed